Skip to content

app: extract the create backup usecase - #1205

Open
huanghaoyuanhhy wants to merge 4 commits into
zilliztech:mainfrom
huanghaoyuanhhy:app/create-usecase
Open

huanghaoyuanhhy wants to merge 4 commits into
zilliztech:mainfrom
huanghaoyuanhhy:app/create-usecase

Conversation

@huanghaoyuanhhy

@huanghaoyuanhhy huanghaoyuanhhy commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Part of #1171

What

app gains the CreateBackup usecase: one struct holding the config, the two storage clients, the task manager and the backup root path, constructor-built from config. The action is create a backup job: a job is what a create call makes, and the backup artifact is what a successful job leaves behind — the two-resource split of the v2 API (#1124), where jobs/backup/create returns the job and backups/describe reads the artifact.

  • Start(req) is the synchronous act of starting a job — it assembles backup.TaskArgs and calls backup.NewTask, which registers the job in the task manager, and returns the registered job ready to run behind a small BackupJob interface
  • Execute(ctx, req) runs the job synchronously and answers with the task manager's view of it: id, state, progress and the rest of the job half — the shape v2's jobs/backup/create responds with. It never reads the persisted meta; a transport whose contract merges the two resources (v1) assembles what it needs itself

Both transports call it:

  • the create CLI command (cmd/create) builds app.NewCreateBackup + Execute; flag parsing, name defaulting, validation and the printed output stay in cmd
  • the /api/v1/create handler (core/server) renders the pb request into app.CreateBackupRequest — keeping the deprecated pb fields (db_collections, collection_names, force, meta_only), name validation and the error-to-code mapping — through a newCreateBackup constructor hook on the server config that tests stub out, the same seam list and delete got in app: extract the list backups usecase #1182 and app: extract the delete backup usecase #1200
  • the v1 async flag stays a server concern: the handler calls Start and spawns the goroutine itself (unchanged logging on failure), while taskmgr registration stays where it was, inside backup.NewTask. core/backup is not touched
  • NewCreateBackup takes the task manager as a parameter instead of reading taskmgr.DefaultMgr() itself; cmd/create and the server wiring pass the process-local manager, keeping the global reference out of the usecase layer

Rebased on top of the config fork (#1215): the v1 backup_root_path field is applied as a config override, not a request field. The handler forks the loaded config with backup.storage.rootPath as one more override layer — the same move get_backup makes for its path parameter in #1203 — and the forked params are what NewCreateBackup builds the usecase from, so CreateBackupRequest carries no root path and the usecase reads it from its params. The fork is reload-equivalent, so unset backup.storage leaves still cascade from milvus.storage, and the server's own config is never mutated. The artifact directory for a given request is the same as before the rebase.

Semantics unchanged

The v1 wire shape is kept, including its quirks:

  • the sync success response still carries only code and msg — no payload and no request_id. The usecase now answers with the job view alone, and the v1 contract needs nothing from it, so the handler discards it; anything a v1 response does need, the handler assembles itself
  • request_id defaulting, error-to-code mapping (Parameter_Error for a bad name, Fail for everything else, request_id echoed on failures), filter/strategy/format precedence and the deprecated-field fallbacks are unchanged
  • async behavior is unchanged: same response message (create backup is executing asynchronously), same request_id echo, task still registered synchronously in the default task manager before the goroutine starts

Two pre-existing bugs were found and are not fixed here:

  • empty backup_name still panics in backup.ValidateName and lands on gin's recovery as a 500, exactly as before

Two deliberate behavioral deltas, both limited to error paths:

  • the sync path used to fall through from a backup.NewTask error into task.Execute on a nil task — a guaranteed panic. The usecase returns that error instead, so a job whose registration fails now answers Fail with the error message
  • the old sync path read the task view, the persisted meta and the meta size after a successful run and answered Fail when any of those reads failed — even though the backup had already succeeded — all to build a brief it then discarded. That read only ever fed the discarded payload, so it is gone together with the payload: a successful backup now always answers success

Tests

  • app/create_test.go: Start registers the job in the task manager and refuses a second live job for the same backup name; Execute propagates the start failure; the artifact dir is the configured root path plus the option's name
  • core/server/create_test.go: httptest cases over the handler contract — sync runs through the usecase and the success response carries no payload (quirk pinned), request_id generated and forwarded, invalid name rejected with Parameter_Error before the usecase runs, constructor/build/execute/start errors mapped to Fail, async starts the job and returns immediately, backup_root_path forks the loaded config while the server's own config stays on the default root path (and without the field the config travels to the usecase untouched); plus the pb→option rendering incl. deprecated force/meta_only and the three filter sources

@sre-ci-robot

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: huanghaoyuanhhy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov-commenter

codecov-commenter commented Sep 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.14094% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.80%. Comparing base (9b4767a) to head (805d333).

Files with missing lines Patch % Lines
app/create.go 48.78% 21 Missing ⚠️
cmd/create/create.go 0.00% 10 Missing ⚠️
core/server/create.go 90.52% 6 Missing and 3 partials ⚠️
core/server/option.go 0.00% 3 Missing ⚠️

❌ Your patch status has failed because the patch coverage (71.14%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.
❌ Your project status has failed because the head coverage (47.80%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1205      +/-   ##
==========================================
+ Coverage   46.89%   47.80%   +0.90%     
==========================================
  Files         140      141       +1     
  Lines       12845    12827      -18     
==========================================
+ Hits         6024     6132     +108     
+ Misses       6386     6255     -131     
- Partials      435      440       +5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mergify mergify Bot added ci-passed and removed ci-passed labels Sep 5, 2026
Signed-off-by: huanghaoyuanhhy <haoyuan.huang@zilliz.com>
Signed-off-by: huanghaoyuanhhy <haoyuan.huang@zilliz.com>
The v1 backup_root_path field used to travel on the request and the
usecase resolved it over the configured root path by hand. Apply it as
one more override layer on the loaded config instead (zilliztech#1215): the
handler forks, the usecase reads the root path from the params it was
built with, and CreateBackupRequest loses its RootPath field. The fork
is reload-equivalent, so unset backup.storage leaves still cascade from
milvus.storage, and the server's own config is never mutated.

Also drop the expectMetaSize copy the rebase onto zilliztech#1203's
app/get_backup_test.go left duplicated in create_test.go.

Signed-off-by: huanghaoyuanhhy <haoyuan.huang@zilliz.com>
A create call makes a job; the backup artifact is what a successful job
leaves behind, read through GetBackup. Execute now returns only the task
manager's view of the job (the shape v2's jobs/backup/create responds
with) and never reads the persisted meta. The v1 sync response keeps its
code+msg-only shape; the old post-success meta read that could turn the
response into Fail is gone with the discarded payload it fed.

Signed-off-by: huanghaoyuanhhy <haoyuan.huang@zilliz.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants